Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cycle chords & chord mapping #22

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Cycle chords & chord mapping #22

merged 4 commits into from
Jun 17, 2024

Conversation

emuell
Copy link
Owner

@emuell emuell commented Jun 14, 2024

Allows using chords in cycles:

cycle("c4'maj g4'min")

and allows mapping notes and/or chords via the map function:

cycle("a b"):map{
  a = "c4'min", 
  b = note{"c4", "d4", "g4"}:with_volume(0.5)
}

Part of #13

@unlessgames I ended up adding a new chord identifier/rule to the grammar instead of using the existing "name" value, as this felt super hacky. The character "'" else also isn't used in the mini notation, so that should be future proof too?

Tidal actually also supports such chord notations, but Strudel doesn't - from what I've seen.


This PR isn't finished yet. I need to deal with lengths and note offs in chords correctly in the event iter impls.

@emuell emuell marked this pull request as ready for review June 14, 2024 17:04
@emuell emuell requested a review from unlessgames June 14, 2024 17:04
@unlessgames
Copy link
Collaborator

I guess eventually it could be better handle this completely inside the mini parser regarding parsing the types of chords and outputting stacks as a result etc.

With regard to the current implementation, the good quirk of the Name way of doing this was that you could append chord info to multiple events at once like <[c c c] [d d] a>:min. Maybe ' should be an operator expression like : to allow for this kind of use?

@emuell
Copy link
Owner Author

emuell commented Jun 14, 2024

<[c c c] [d d] a>:min

This would have been parsed as Pitch + Target - and still is? So using Name instead of the new Chord doesn't change anything here.

As long as we're not breaking basic tidal compatibility, either way is fine for me.

But yes, using ' as a new operator would indeed be the cleanest solution, but I'm not really keen to duplicate all the chord parsing functionality in the cycle. You'd also have to extend the parser to handle all kinds of supported chords, which seems awkward.


Apropos: I'm not a fan of the tidal `ii chord inversion notation. I find it quite confusing and limiting, but that probably should be supported sometime in the future as well...

@emuell emuell force-pushed the feature/cycle-chord-mapping branch from b32ffd5 to ce5a8d1 Compare June 15, 2024 19:58
@emuell
Copy link
Owner Author

emuell commented Jun 16, 2024

@unlessgames to merge or not to merge.

If you dislike the chord changes in the grammar, I'm fine with that and will parse that from the name. I don't think it's causing any harm and can later still be finetuned.

@unlessgames
Copy link
Collaborator

Sorry, I don't dislike the changes, was just still wondering if somehow extending the Target system to include chords was a better start, but probably not. This already works so let's merge.

@emuell emuell merged commit 2a81635 into master Jun 17, 2024
1 check passed
@emuell emuell deleted the feature/cycle-chord-mapping branch June 17, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants